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

[Christina Fu]

Endi,
Thanks for the review comments.
Attached please find the patch that addressed most your comments except 
for the following which we could discuss further, if needed:
1. per our irc discussion, we'll leave the changes in CS.cfg alone for now
5. The target gets reset at beginning at each iteration, isn't that what 
we want?
6. Indeed, it does fail at installation.  I took a look and find the 
attached changes in the patch to be working.  However, I don't have 
anything to be substituted for so I am not sure if slot_substitution.py  
is the right place to put it to copy, but it worked for me.
I did try another location in subsystem_layout.py, but it failed. Maybe 
someone with more experience with Python and installation scripts would 
know.  Anyway,, again, the changes in this patch seems to work for me.

thanks,
Christina

On 06/03/2014 11:47 AM, Endi Sukma Dewata wrote:
> 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.
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-cfu-0013-TPS-Token-Profile-Resolver-Framework-part2.patch
Type: text/x-patch
Size: 5043 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20140603/f90e55b9/attachment.bin>