[Pki-devel] <pki-devel> [PATCH] 0005- Further work on TPS Processor, format operation.patch

[Endi Sukma Dewata]

On 3/17/2014 9:18 PM, John Magne wrote:
> Revised patch to address irc comments:
>
> Mostly concerning error handling and a new way to declare the TPSEngine class.

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?

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.

3. The cuid_x variable in getTokenType() doesn't seem to be necessary. 
It looks like it can use cuid directly.

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);
   }

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(",")) {
       ...
   }

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);
   }

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.

8. The following lines can be combined:

   int major = 0;
   major = Integer.parseInt(majorVersion);

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;
   }

-- 
Endi S. Dewata

> ----- Original Message -----
>> From: "John Magne" <jmagne at redhat.com>
>> To: "pki-devel" <pki-devel at redhat.com>
>> Sent: Thursday, March 13, 2014 4:29:17 PM
>> Subject: [Pki-devel] <pki-devel> [PATCH] 0004- Further work on TPS Processor, format operation.patch
>>
>> 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.