[Pki-devel] <pki-devel> [PATCH] 0006- Further work on TPS Processor, format operation.patch
John Magne
jmagne at redhat.com
Wed Mar 19 19:03:03 UTC 2014
cfu, thanks for responses.
My comments below:
----- Original Message -----
> From: "Christina Fu" <cfu at redhat.com>
> To: pki-devel at redhat.com
> Sent: Wednesday, March 19, 2014 11:04:19 AM
> Subject: Re: [Pki-devel] <pki-devel> [PATCH] 0006- Further work on TPS Processor, format operation.patch
>
> I checked areas addressing my comments.
>
> A couple things:
> * those logging/audit related log parameter name defines will eventually have
> to follow the other subsystems' param name style in CS.cfg (nothing to be
> done.)
>
Yes, agreed.
> * I think at some point, we (both of us) have to define those exception
> messages in those properties file shared by other subsystems.
> Also, I'm not certain I like the introduction of TPSException. If
> EBaseException is not enough to serve your purpose, you should make it a
> more generic exception and put it at a place and make it available for all
> subsystems.
cfu, TPSException is kind of specific to TPS since it carries a TPS specific
error code that goes back to the client. It is true that EBaseException has
a facility to add some generic object "parameters" of some kind, I though the
new class might add some convenience and clarity. We can argue it going forward though.
> (I'll leave it up to you, maybe consult Endi)
>
> * I might have missed it. I see you removed the session== null check but I
> don't see you putting it back near the top of format().
cfu, On this one Endi had me to a null check on the session when creating the
processor object constructor.
>
> Consider it an ACK if you have addressed 3rd comment, and possible 2nd.
>
> Christina
>
>
> On 03/18/2014 07:03 PM, John Magne wrote:
>
>
>
> Have addressed cfu's and edewatas comments in attached patch as per below:
>
> Prefix feature for some reason isn't working on the email. Comments encclosed
> in: <jack> comment </jack>
>
> Edewata's comments:
>
>
>
>
> Some comments/questions:
>
> 1. Is the TPSFormatProcessor still needed since the TPSSession can use
> TPSProcessor directly? Or maybe should TPSProcessor.format() be moved
> into TPSFormatProcessor?
>
> <jack>
> As per you and cfu, this one is history. We can always bring
> it back if we need to. The other processors to come will have significant
> content.
> </jack>
>
>
>
> 2. As discussed over IRC, the TPSProcessor.format() right now is using
> two mechanisms to return the status: via exception and via return value.
> Will it be changed to use one mechanism in the future? Also already
> discussed, the code that checks the return value of the check*() method
> can actually be moved into the check*() method itself, then throw an
> TPSException with the status if it fails the validation.
>
> <jack>
> Done, now we only return an exception. Also validations are
> inside check* methods when appropriate, based on intent of method.
> </jack>
>
> 3. The cuid_x variable in getTokenType() doesn't seem to be necessary.
> It looks like it can use cuid directly.
>
> <jack>
> Done
> </jack>
>
> 4. In the following code, the getString() will assign a default value if
> the property is missing, so the exception will not happen for missing
> property.
>
> try {
> mappingOrder = configStore.getString(configName, null);
> } catch (EBaseException e1) {
> //The whole feature won't work if this is wrong.
> throw new TPSException(
> "TPSProcessor.getTokenType: Token Type configuration
> incorrect! Mising mapping order!",
> TPSStatus.STATUS_ERROR_DEFAULT_TOKENTYPE_NOT_FOUND);
> }
>
> <jack>
> Done.
> </jack>
>
>
>
>
> The exception could still happen for other errors, but in that case the
> exception message above will be incorrect.
>
> 5. The loop can be simplified like this:
>
> for (String mappingId : mappingOrder.split(",")) {
> ...
> }
>
> <jack>
> Done
> </jack>
>
>
>
> 6. Similar to #4, the following code will catch an incorrect type of error:
>
> try {
> targetTokenType = configStore.getString(mappingConfigName, null);
> } catch (EBaseException e) {
> throw new TPSException(
> "TPSProcessor.getTokenType: Token Type configuration
> incorrect! Invalid or no targetTokenType.",
> TPSStatus.STATUS_ERROR_DEFAULT_TOKENTYPE_NOT_FOUND);
> }
>
> <jack>
> Done
> </jack>
>
>
>
>
> 7. Similar to #4, the following code will assign a default value if the
> property is missing. If there's an exception, it must have been caused
> by something else, not because of the missing property. In that case
> it's better to break and report the error.
>
> try {
> tokenType = configStore.getString(mappingConfigName, null);
> } catch (EBaseException e) {
> tokenType = null;
> }
>
> try {
> tokenATR = configStore.getString(mappingConfigName, null);
> } catch (EBaseException e) {
> tokenATR = null;
> }
>
> try {
> tokenCUIDStart = configStore.getString(mappingConfigName, null);
> } catch (EBaseException e) {
> tokenCUIDStart = null;
> }
>
> try {
> tokenCUIDEnd = configStore.getString(mappingConfigName, null);
> } catch (EBaseException e) {
> targetTokenType = null;
> }
>
> try {
> majorVersion = configStore.getString(mappingConfigName, null);
> } catch (EBaseException e) {
> majorVersion = null;
> }
>
> try {
> minorVersion = configStore.getString(mappingConfigName, null);
> } catch (EBaseException e) {
> minorVersion = null;
> }
>
> The code should either wrap EBaseException with TPSException and throw
> it, or alternatively declare EBaseException in the getTokenType() and
> remove the try-catch block.
>
> <jack>
> Done. Each exception in routine is handled based on whether we can
> tolerate a default.
> </jack>
>
>
>
>
>
> 8. The following lines can be combined:
>
> int major = 0;
> major = Integer.parseInt(majorVersion);
>
> <jack>
> Done
> </jack>
>
>
>
>
> Same thing for the minor version.
>
>
>
> 9. There are checks in TPSProcessor to make sure the session is not
> null. This is no longer necessary since the session is supplied when
> creating the instance. If validation is necessary, it can be done just
> one time in the constructor itself:
>
> public TPSProcessor(TPSSession session) {
> if (session == null) throw NullPointerException("TPS session is
> null");
> this.session = session;
> }
>
>
> <jack>
> Done
> </jack>
>
>
>
> CFU's comments:
>
> I find it easier to just apply the patches to the tree to have a better
> context. Some of the things I've found may be from the past.
>
> * TPSEngine -
> - you have defined a number TKS_RESPONSE_xxx constants which I have defined
> in IRemoteRequest.java. I think IRemoteRequest.java is a better place
> since I have TKS server side using those constants as well. If agreed,
> how about removing those 6 constants from TPSEngine? I checked and i see
> in your code you are not using them yet anyway, so it should not be too
> much trouble.
>
> <jack>
> Done.
> </jack>
>
>
>
> * TPSFormatProcessor
> - In our old tps, I have always questioned the existance of the
> RA_Format_Processor as well as the other xxx_Processor's other than the
> RA_Processor.cpp and RA_Enroll_Processor.cpp where the real code were in.
> My question is, is there going to be an attempt of putting real format code
> into the TPSFormatProcessor, etc, or we are merely going to do a direct
> conversion of C++ to Java? The reason why I ask is because the
> TPSFormatProcessor and all probably serve not much purpose unless we put
> something useful in them. We can probably leave them as is for now and
> think about it later. I just want to bring it up so we can keep it in
> mind to ponder on.
>
> <jack>
> Done for both cfu and edewata.
> </jack>
>
> * Would it make sense for APDUResponse to carry a boolean status so that
> checkTokenPDUResponse() only needs to be processed once and status set at
> the time? Anyway, if there is concern of veering off to far from the old
> tps course then we can keep the return rc style.
>
> <jack>
> Done. Here I moved the method to get a boolean result into the APDUResponse
> class. Also added convenience class to get the two byte result for future
> use.
> </jack>
>
>
> * I don' t see in the format() where you pass in the skipAuth value.
>
> <jack>
> The original code didnt seem to be using this. If we determine otherwise, it
> can be fixed later.
> I suspect this is now controlled strictly by config.
> </jack>
>
>
>
>
> * I may have missed it, but I don't see you checking if cplc_data < 47 (a
> magic number in old tps).
>
> <jack>
> Done. I was checking this value in the subsequent methods that fish strings
> out of the cplc_data.
> Now have the check in cplc_data specific routine.
> </jack>
>
>
>
> * why do you change the variable name token_status to status? I think
> token_status, or if you wish, tokenStatus, is more descriptive.
>
> <jack>
> Done
> </jack>
>
> * why do you check if (session == null) in the middle of assigning major and
> minor versions? Could you not have checked this earlier, like near the
> beginning of format()?
>
> Done. Must have been some mystery fantom paste going on there.
>
> * the old tps might be wrong, but I just want to know why is it that it would
> assign 0 to all those versions if token_status == NULL, while you bumped it
> out?
>
> I believe this is the way it works. If there is no applet installed, there
> is no status.
>
>
>
> * perhaps we want to consider keeping the tokenType somewhere we can retrieve
> (not re-calculated and re-retrieved) easily during a session, instead of
> passing it around as a parameter. Where is a good place to do that?
>
> <jack>
> Good idea, done.
> </jack>
>
> that's it for now.
> Christina
>
> On 03/13/2014 04:29 PM, John Magne wrote:
>
> Ticket #895 https://fedorahosted.org/pki/ticket/895 Further work on TPS
> Processor, format operation.
>
> This patch gets a bit farther on the TPS format operation, just before
> applet upgrade, which will also need secure channel functionality.
>
> Also, patch provides some misc clean up and functionality.
>
> 1. Method to calculate the token type.
> 2. Some added convenience methods to get various config params for the
> Format operation.
> 3. More progress for the format operation up until we attempt to upgrade
> the applet.
> 4. Added TPSException that holds a message and end op return code. Can be
> used to throw from anywhere and the return code makes it back to the
> client.
>
>
> _______________________________________________
> Pki-devel mailing list Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
>
>
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
More information about the Pki-devel
mailing list