Implement v2 of AgentCertRequestService witout VLV and resteasy #4672
Conversation
Since v2 rest API are implemented without JAX-RS support the interceptors used in current implementation for the authentication have been converted to filters maintaining very similar functionality. The only difference is in the audit log because it will not be logged the access method but the accessed REST URI.
The GET method allow to list the cert requests and retrieve one for review. The main difference with the old implementation is that the start parameter is not anymore a request id but an index in the list of requests.
The POST methods allow all the operation on the certrequest. These are the same available in the previous implementation and support the same parameters.
|
@edewata I have done some test enabling the client and they worked as expected. Before to move the client on v2 the start parameter has to be modified (at least the documentation) or, if for some reason the previous approach could be useful even with random ids, make the start an id. We can handle this step in a following commit. |
There was a problem hiding this comment.
Please see my comments below. I think generally the code is consistent with the original v1 code. The only issue is the placement of RequestNotFoundException, but the rest are just suggestions for improvements that can be done separately later. Feel free to update & merge. Thanks!
| try { | ||
| reqInfos.addEntry(CertRequestInfoFactory.create(request)); | ||
| } catch (NoSuchMethodException e) { | ||
| logger.warn("Error in creating certrequestinfo - no such method: " + e.getMessage(), e); | ||
| } |
There was a problem hiding this comment.
It looks like CertRequestInfoFactory.create() doesn't actually need to throw anything so we could remove this try-catch block.
| return reqInfos; | ||
| } | ||
|
|
||
| private String createSearchFilter(String requestState, String requestType) { |
There was a problem hiding this comment.
This method and the one in AgentCertRequestService v1 probably can be consolidated into CertRequestDAO.
There was a problem hiding this comment.
Not sure CertRequestDAO is needed. This will depend on the other service we have to move. We can decide later.
| return filter; | ||
| } | ||
|
|
||
| private void changeRequestState(RequestId id, HttpServletRequest request, CertReviewResponse data, |
There was a problem hiding this comment.
It might be possible to reuse the CertRequestDAO.changeRequestState().
There was a problem hiding this comment.
Yes, the idea was to not use classes from rest folder so we can deprecate/drop the folder. If needed we can move these utility classes in a separate folder to use for multiple API versions. This is the case if the same method is needed for other service.
| import com.netscape.cmscore.authorization.AuthzSubsystem; | ||
| import com.netscape.cmscore.logging.Auditor; | ||
|
|
||
| public abstract class ACLFilter extends GenericFilter { |
There was a problem hiding this comment.
Should we use HttpFilter instead of GenericFilter?
|
|
||
| @Override | ||
| public void init() throws ServletException { | ||
| super.init(); |
There was a problem hiding this comment.
It's actually not necessary to call super.init():
https://javaee.github.io/javaee-spec/javadocs/javax/servlet/GenericFilter.html#init--
|
|
||
| @Override | ||
| public void init() throws ServletException { | ||
| super.init(); |
| import org.dogtagpki.server.v2.ACLFilter; | ||
|
|
||
| @WebFilter(servletNames = "caCertRequest-agent") | ||
| public class AgentCertRequestACLFilter extends ACLFilter { |
There was a problem hiding this comment.
IIUC this means we will need to create quite a few WebFilter classes for all v2 servlets we're going to have, and each filter instance will have its own copy of aclProperties or authProperties. That should work fine but probably redundant.
I think alternatively we could probably define a subclass of each filter for the whole subsystem (e.g. CAACLFilter and CAAuthMethodFilter), then specify all URLs in that subsystem that the filters should apply to, then probably reuse the existing @ACLMapping and @AuthMethodMapping annotations to define the ACL and auth method for each servlet.
There was a problem hiding this comment.
Using the same approach has a problem. Using jax-rs the interceptor take control after the routing so it is possible to know which method will be used. Here, filters know only which servlet is invoked but not the method so it is not easy to have the same ACL granularity. If we want to define an ACL on each operation then we need to evaluate the path for all the servlet or, alternatively, we need to write a routing utility which works like resteasy. I think we have to evaluate this when we have more servlet implemented.
By the way, we need to have a filter for all the servlet if we want the audit of the event even no ACL are applied so we need a filter for the servlet we have already implemented and which do not require authentication.
| if (info == null) { | ||
| // request does not exist | ||
| throw new RequestNotFoundException(id); | ||
| } |
There was a problem hiding this comment.
I think CertReviewResponseFactory.create() cannot return a null value so this check is not needed.
This code probably should be moved here:
CertReviewResponse req = getRequestData(request, id);
if (req == null) {
// request does not exist
throw new RequestNotFoundException(id);
}
out.println(req.toJSON());
|
@edewata Thanks! I have updated following your comments and merged. |
Implement the AgentCertRequestService using a plain servlet and without VLV. The interface is very similar but the start parameter when retrieving the list is not anymore the id to start from but the index to start. Using random indexes it is less useful to start from a specific attribute.
The authN/authZ was performed using interceptor but in a plain servlet these have been converted in filters maintaining the same behaviour.