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

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.