Re: [Pki-devel] PATCH 34-1 - Restful interface to create certificate requests
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.
Attachments:
- pki-vakwetu-0034-2-Adding-restful-interface-to-create-certificate-reque.patch (text/x-patch — 305.3 KB)