[Pki-devel] PATCH 34-1 - Restful interface to create certificate requests

[Ade Lee]

I have attached an updated patch to address the comments (see below).
In addition, the patch:
* modifies the restful interface for approve/reject etc.
from /certrequest/approve to /certrequest/{id}/approve
* removes an unneeded class and some unneeded annotations.
* changes the name of one class (PolicyAttribute) to ProfileAttribute
  and uses this class in new jaxb model classes for ProfileOutputs
* adds ProfileOutputs to the AgentEnrollmentRequestData class.
* Refactors the ProfileProcess servlet to use the ProfileProcessor.

Please review.  The attached patch replaces the existing patch.

Thanks,
Ade
 
On Thu, 2012-06-28 at 09:52 -0500, Endi Sukma Dewata wrote:
> On 6/26/2012 3:47 PM, Ade Lee wrote:
> > Please review.
> >
> > Tests done:
> > 1. Cert issuance and approval from UI (manual, maul dual cert, agent
> > authenticated server cert)
> > 2. Cert issuance in installation of other subsystems
> > 3. Cert issuance (user and server) from RA.
> > 4. Cert issuance and approval from restful interface using CATest
> > -- two step (create cert request/ agent approval) user and server certs
> > -- single step (agent approved user and server certs)
> 
> Some comments, more to follow. Some of these might have been discussed 
> previously.
> 
> 1. Some fields in the ProfileProcessor are initialized to null in the 
> field declaration. This is not necessary because it's null by default.
> 
done.

> 2. The code in ProfileProcessor.java:1450 might fail because statEvents 
> could still be null if an error happens before the first startTiming() 
> is called. It might be better to initialize statEvents in the field 
> declaration instead of in startTiming(). Alternatively, it's possible to 
> track the timing without statEvents using a separate try-finally block 
> for each startTiming() & and endTiming() pair:
> 
>    startTiming("enrollment");
>    try {
>        ...
>        startTiming("request_population");
>        try {
>            ...
>        } finally {
>            endTiming("request_population");
>        }
>        ...
>    } finally {
>        endTiming("enrollment");
>    }
> 
fixed. initialized in the field declaration.

> 3. Not sure if this is a problem. In ProfileProcessor.java:721 the 
> errorCode is initialized to null before the loop. Inside the loop the 
> errorCode is set when there's an exception but never reset to null 
> again. So if one request fails and the next one succeeds it will still 
> have the errorCode of the previous failure. The errorCode determines 
> whether to call markAsServiced() or updateRequest(). The original 
> servlet has the same behavior.
> 

This is tricky -- the behavior is less than ideal but I specifically
left it this way because thats what the original servlet did.  We can
discuss if/how we should fix it.

> 4. The serial_num in ProfileProcessor.java:412 seems to be used for 
> renewal only. It might be better to move it into processRenewal().
> 
done.

> 5. In ProfileProcessor the ps.getProfile() is called multiple times to 
> get the profile. Usually it's using the profileId specified in the 
> request (line 415) but it can be overriden by the configuration (lines 
> 460 & 555). However, the profile inputs are always obtained from the 
> profile specified in the request (line 417). So it's possible the inputs 
> won't match the profile. It might be better to check the configuration 
> when creating the EnrollmentRequestData then subsequent code will use 
> the one specified in it.
> 
good catch. done.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-vakwetu-0034-2-Adding-restful-interface-to-create-certificate-reque.patch
Type: text/x-patch
Size: 312593 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20120629/3c92bee9/attachment.bin>