TimeStampCheck.java: - Can you please inline all those private getXyz() calls in Interceptor into getRespParam()? I would like to see all these customizations in one place. Just add a blank line in between and it will be clean enough for me.
TsaParam.java: - Please group the fields into different area. Looks like some are fields in TSTInfo, and some are server internals. The overall design is good, but I have a feeling that the interface and implementation can be further separated. I can see that there are a lot of methods and classes that will not be touched by TimeStampCheck at all. Should these things be taken out to be another implementation which is parallel to the one in TimeStampCheck? Maybe TsaHandler::createSigner will be abstract. There are quite some public constructors in the classes. Are they meant to be called by a user? Or by a child class? Or just internal? Also, for those protected methods (Ex: parseRequestParam and createResponse in TsaSigner), do you meant to override it in the future? Can you give examples on how to use this server? Will it always use DefaultRespInterceptor? If so, will the default impl in RespInterceptor.java ever be used? Thanks, Max > On Jun 2, 2020, at 10:05 PM, sha.ji...@oracle.com wrote: > > After discussed with Max, I just updated the patch, > http://cr.openjdk.java.net/~jjiang/8244683/webrev.04/ > > Interface TsaInterceptor is renamed to RespInterceptor. The methods, which > affect TSA response fields, are merged into getRespParam(reqParam). > > Best regards, > John Jiang > > On 2020/5/13 08:18, sha.ji...@oracle.com wrote: >> Hi Max, >> Thanks for your comments! >> Please review the updated webrev: >> http://cr.openjdk.java.net/~jjiang/8244683/webrev.02/ >> The codes are refactored significantly. >> >> On 2020/5/11 10:51, Weijun Wang wrote: >>> Can you update the existing TimeStampCheck test to use this class? I know >>> that test can simulate some error conditions. Maybe you can add one or more >>> virtual methods in this class so TimeStampCheck can override them. >> This test is updated to use this TSA server. >> A new introduced class, namely TsaInterceptor, defines some extension points >> for the signing. >> >>> getDefaultSigAlgo(): Please call AlgorithmId.getDefaultSigAlgForKey() >>> instead. It will be enhanced to support new algorithm. >> Fixed. >> >>> Param: How about making it a JDK 14 record? >> In the updated webrev, this class has changed to TsaParam. >> The fields are not final, and especially this class could be extended by >> tests. >> With my understanding, this language feature may not be applicable for this >> scenario . >> >> Best regards, >> John Jiang >>> Thanks, >>> Max >>> >>>> On May 11, 2020, at 9:28 AM,sha.ji...@oracle.com wrote: >>>> >>>> Hi, >>>> This patch introduces a TSA server, which can work with jarsigner. >>>> This server will be used by the following jar signing tests. >>>> >>>> Issue:https://bugs.openjdk.java.net/browse/JDK-8244683 >>>> Webrev:http://cr.openjdk.java.net/~jjiang/8244683/webrev.00/ >>>> >>>> Best regards, >>>> John Jiang >>>>
