3:47 p.m.
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)
Ade
9:52 a.m.
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.
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");
}
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.
4. The serial_num in ProfileProcessor.java:412 seems to be used for
renewal only. It might be better to move it into processRenewal().
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.
--
Endi S. Dewata
11:47 p.m.
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.
10:29 a.m.
On 6/28/2012 11:47 PM, Ade Lee wrote:
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.
More comments:
6. The ProfileProcessor is very big creating maintenance issue. It
probably can be split into smaller classes that handle specific actions,
e.g. CertEnrollmentProcessor, CertRenewalProcessor,
CertApprovalProcessor. Common fields & methods could be moved into a
Processor base class.
7. The DAO layer doesn't seem to be necessary because it used
exclusively by the REST service and the majority of work is done in
Processor layer. I guess when we merge the plural and singular REST
service later we can merge DAO too.
8. The ProfileProcessor can use the Auditor service to simplify the
auditing code. Is this going to be done separately?
9. In ProfileServlet the statEvents is initialized in startTiming. It's
safer to initialize it in the field declaration as in ProfileProcessor.
10. To be consistent XML element name should be capitalized and XML
attribute names should be lower case, for example:
<PolicyDefault id="...">
<Description>...</Description>
</PolicyDefault>
11. Since REST data model is going to be shared with the client, it
should not depend on classes that are used for the server only. This is
not a problem now because certserv package contains both shared and
server-only classes, but they will be difficult to separate later if the
shared classes depend on server-only classes. For example PolicyDefault
is a REST data model, but it depends on IRequest which is most likely a
server-only interface. It might be better to have a separate factory
class (e.g. PolicyDefaultFactory) which can construct a PolicyDefault
object and then set its attributes with values obtained from server-only
objects.
12. Formatting issue in CARestClient.java:125, PolicyConstraint.java:41.
--
Endi S. Dewata
8:34 a.m.
New patch attached. This patch replaces the previous one.
See comments below.
Ade
On Fri, 2012-06-29 at 10:29 -0500, Endi Sukma Dewata wrote:
On 6/28/2012 11:47 PM, Ade Lee wrote:
> 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.
More comments:
6. The ProfileProcessor is very big creating maintenance issue. It
probably can be split into smaller classes that handle specific actions,
e.g. CertEnrollmentProcessor, CertRenewalProcessor,
CertApprovalProcessor. Common fields & methods could be moved into a
Processor base class.
Done. See patch.
7. The DAO layer doesn't seem to be necessary because it used
exclusively by the REST service and the majority of work is done in
Processor layer. I guess when we merge the plural and singular REST
service later we can merge DAO too.
Agreed.
8. The ProfileProcessor can use the Auditor service to simplify the
auditing code. Is this going to be done separately?
Yes - there is already a task to handle this.
9. In ProfileServlet the statEvents is initialized in startTiming.
It's
safer to initialize it in the field declaration as in ProfileProcessor.
Done.
10. To be consistent XML element name should be capitalized and XML
attribute names should be lower case, for example:
<PolicyDefault id="...">
<Description>...</Description>
</PolicyDefault>
Separate task. We need to choose a standard and use it consistently
across the interface.
11. Since REST data model is going to be shared with the client, it
should not depend on classes that are used for the server only. This is
not a problem now because certserv package contains both shared and
server-only classes, but they will be difficult to separate later if the
shared classes depend on server-only classes. For example PolicyDefault
is a REST data model, but it depends on IRequest which is most likely a
server-only interface. It might be better to have a separate factory
class (e.g. PolicyDefaultFactory) which can construct a PolicyDefault
object and then set its attributes with values obtained from server-only
objects.
Good idea. Done.
12. Formatting issue in CARestClient.java:125,
PolicyConstraint.java:41.
Fixed.
1:03 p.m.
Moved and renamed processor classes as specified in #irc with Endi.
The new structure is :
Processor -> CertProcessor -> <Request|Enrollment|Renewal|
Revocation>Processor
Acked by Endi.
Pushed to master. Attaching patch for completeness.
On Mon, 2012-07-02 at 09:34 -0400, Ade Lee wrote:
New patch attached. This patch replaces the previous one.
See comments below.
Ade
On Fri, 2012-06-29 at 10:29 -0500, Endi Sukma Dewata wrote:
> On 6/28/2012 11:47 PM, Ade Lee wrote:
> > 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.
>
> More comments:
>
> 6. The ProfileProcessor is very big creating maintenance issue. It
> probably can be split into smaller classes that handle specific actions,
> e.g. CertEnrollmentProcessor, CertRenewalProcessor,
> CertApprovalProcessor. Common fields & methods could be moved into a
> Processor base class.
>
Done. See patch.
> 7. The DAO layer doesn't seem to be necessary because it used
> exclusively by the REST service and the majority of work is done in
> Processor layer. I guess when we merge the plural and singular REST
> service later we can merge DAO too.
>
Agreed.
> 8. The ProfileProcessor can use the Auditor service to simplify the
> auditing code. Is this going to be done separately?
>
Yes - there is already a task to handle this.
> 9. In ProfileServlet the statEvents is initialized in startTiming. It's
> safer to initialize it in the field declaration as in ProfileProcessor.
>
Done.
> 10. To be consistent XML element name should be capitalized and XML
> attribute names should be lower case, for example:
>
> <PolicyDefault id="...">
> <Description>...</Description>
> </PolicyDefault>
>
Separate task. We need to choose a standard and use it consistently
across the interface.
> 11. Since REST data model is going to be shared with the client, it
> should not depend on classes that are used for the server only. This is
> not a problem now because certserv package contains both shared and
> server-only classes, but they will be difficult to separate later if the
> shared classes depend on server-only classes. For example PolicyDefault
> is a REST data model, but it depends on IRequest which is most likely a
> server-only interface. It might be better to have a separate factory
> class (e.g. PolicyDefaultFactory) which can construct a PolicyDefault
> object and then set its attributes with values obtained from server-only
> objects.
>
Good idea. Done.
> 12. Formatting issue in CARestClient.java:125, PolicyConstraint.java:41.
>
Fixed.
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
4313
days inactive
4320
days old
5 comments
2 participants
participants (2)
-
Ade Lee -
Endi Sukma Dewata