[Pki-devel] [PATCH] pki-cfu-0009-TPS-Token-Profile-Resolver-Plugin-Framework-Ticket-4.patch

[Endi Sukma Dewata]

On 6/2/2014 4:40 PM, Christina Fu wrote:
> pushed to master
> commit 8f6103382199357ede73c02e355a09bcf7e41b8d
>
> In case Endi has more to add, I'm attaching the latest patch checked in
> and keeping the ticket open.
>
> Christina

Some comments:

1. In the CS.cfg the display name "Profile Mapping" is replaced with 
"Token Profile Mapping Resolvers" and "Profile" is replaced with "Token 
Profile". Do we need to do the same changes in the UI?

2. The TokenProfileParams.getString() generates 
DEFAULT_TOKENTYPE_NOT_FOUND if the attribute is missing. Should it be 
DEFAULT_TOKENTYPE_PARAMS_NOT_FOUND instead?

3. Same issue with TokenProfileParams.getInt() as in #2.

4. TokenProfileParams.getInt() catches NumberFormatException and 
generates DEFAULT_TOKENTYPE_NOT_FOUND, which is misleading. Is there a 
more appropriate error code for invalid attribute value? Or it's 
probably better not to catch the NumberFormatException.

5. This is probably an existing issue, but it looks like there's a logic 
problem in MappingTokenProfileResolver.getTokenType(). See the inline 
comments:

   // targetTokenType is initially null
   String targetTokenType = null;

   // iterate through all mappings
   for (String mappingId : mappingOrder.split(",")) {

       // assign targetTokenType value
       targetTokenType = configStore.getString(mappingConfigName);

       // do various validations
       // if validation fails, go to the next mapping
       if (tokenType != null && tokenType.length() > 0) {
           if (eTokenType == null) {
               continue;
           }

           if (!eTokenType.equals(tokenType)) {
               continue;
           }
       }

       // if we make it this far, we have a token type
       break;
   }

   // if targetTokenType is null, throw exception.
   if (targetTokenType == null) {
       throw new TPSException(...);
   }

   return targetTokenType;

The targetTokenType is assigned a value early in each iteration. If the 
validation fails, the targetTokenType carries over the value to the next 
iteration. Let's suppose all validations fail, the targetTokenType will 
still have a value from the last iteration, which is probably not what 
we wanted. I think targetTokenType should only be assigned a value after 
all validations have passed (i.e. "if we make it this far...").

6. Has this been tested? I tried installing TPS with the latest code in 
master and it failed. Other subsystems can be installed just fine.

-- 
Endi S. Dewata