Jack, Thanks for the review comments. Attached please find the new patch that addressed your comments, both on-line and off-line. Christina On 05/30/2014 05:06 PM, John Magne wrote:

Couple of initial comments on this after looking over it:

1.

Your TokenProfileParams class has this hash map.

private HashMap<String, Object> content = new HashMap<String, Object>();

Then when we add items and get items from this thing, we have to do some fancy footwork to account for the fact that the "Object" part is holding either a String value or an int value.

My opinion after doing some research is that this is kind of awkward.

A couple of possible solutions:

1. Have the HashMap hold <String,String>.

When we want an int we just convert back and forth from string. This is done often when processing http name value pairs and such.

2. Instead of relying upon the "get" method of the hash map to return encoded values, maybe have the parent object "TokenProfileParams" have two separate methods getString() and getInt(). Each one would take the key name and output the proper value, int or String

2. We have the following abstract base class:

public abstract class BaseTokenProfileResolver

The abstract class sounds like a good way to have some base functionality to be used by derived classes. I also believe that the idea is to declare some abstract methods that forces derived classes to implement these methods.

In this case it sounds like the following method would be a good candidate for abstract:

public String getTokenType(TokenProfileParams pPram);

----- Original Message -----

...

From: "Christina Fu" <cfu@redhat.com> To: pki-devel@redhat.com Sent: Friday, May 30, 2014 3:07:56 PM Subject: [Pki-devel] [PATCH] pki-cfu-0009-TPS-Token-Profile-Resolver-Plugin-Framework-Ticket-4.patch

This patch implements a Token Profile Resolver Plugin Framework for TPS. It allows a site to implement its own token profile resolver.

The TokenProfileResolverManager initializes the token Resolver plugin framework from the following configuration files: - <instance patch>/conf/registry.cfg - <instance patch>/conf/CS.cfg

The existing mapping method "getTokenType" was extracted into the default plugin code: MappingTokenProfileResolver. All resolver plugin codes will extend from BaseTokenProfileResolver.

The most visible change from sites running previous versions of TPS is that the mapping related config params e.g. op.format.mapping.xxx have now being extracted into token resolver instance params e.g. tokenProfileResolver.formatMappingResolver.mapping.xxxx where "class_id" defines the plugin implementation name defined in the registry.cfg and op.format.tokenProfileResolver points to the defined resolver instance e.g. op.format.tokenProfileResolver=formatMappingResolver

A separate ticket will be filed to provide actual plugin writing instruction as well as a sample plugin.

Finally, even though this patch does not directly address https://fedorahosted.org/pki/ticket/447 RFE: Mapping tokens to tokentype, where the request was to figure out the tokenType (Token profile) by uid instead of the current mapping method, it provides the means for one to write any customized plugins.

thanks, Christina

_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

ACK. This looks good. Just one thing to fix in my opinion before submitting and or edewata wants to have a look too. The method: protected String resolveTokenProfile(String resolverInstName, String cuid, String msn, byte major_version, Reads in a bunch of params and catches any exception and then forces the returned token type to be "userKey". I think that maybe if the admin is trying to get a certain result and then the code silently moves on with "userKey", it might be confusing. It seems to be ok to simply throw the exception there and they can debug it and get it correctly set. ----- Original Message -----

From: "Christina Fu" <cfu@redhat.com> To: pki-devel@redhat.com Sent: Monday, June 2, 2014 12:20:28 PM Subject: Re: [Pki-devel] [PATCH] pki-cfu-0009-TPS-Token-Profile-Resolver-Plugin-Framework-Ticket-4.patch

This newer patch added the getInt() and getString() methods per suggestion of Jack.

Please review. thanks, Christina

On 06/02/2014 11:14 AM, Christina Fu wrote:

Jack,

Thanks for the review comments. Attached please find the new patch that addressed your comments, both on-line and off-line.

Christina

On 05/30/2014 05:06 PM, John Magne wrote:

Couple of initial comments on this after looking over it:

1.

Your TokenProfileParams class has this hash map.

private HashMap<String, Object> content = new HashMap<String, Object>();

Then when we add items and get items from this thing, we have to do some fancy footwork to account for the fact that the "Object" part is holding either a String value or an int value.

My opinion after doing some research is that this is kind of awkward.

A couple of possible solutions:

1. Have the HashMap hold <String,String>.

When we want an int we just convert back and forth from string. This is done often when processing http name value pairs and such.

2. Instead of relying upon the "get" method of the hash map to return encoded values, maybe have the parent object "TokenProfileParams" have two separate methods getString() and getInt(). Each one would take the key name and output the proper value, int or String

2. We have the following abstract base class:

public abstract class BaseTokenProfileResolver

The abstract class sounds like a good way to have some base functionality to be used by derived classes. I also believe that the idea is to declare some abstract methods that forces derived classes to implement these methods.

In this case it sounds like the following method would be a good candidate for abstract:

public String getTokenType(TokenProfileParams pPram);

----- Original Message -----

From: "Christina Fu" <cfu@redhat.com> To: pki-devel@redhat.com Sent: Friday, May 30, 2014 3:07:56 PM Subject: [Pki-devel] [PATCH] pki-cfu-0009-TPS-Token-Profile-Resolver-Plugin-Framework-Ticket-4.patch

This patch implements a Token Profile Resolver Plugin Framework for TPS. It allows a site to implement its own token profile resolver.

The TokenProfileResolverManager initializes the token Resolver plugin framework from the following configuration files: - <instance patch>/conf/registry.cfg - <instance patch>/conf/CS.cfg

The existing mapping method "getTokenType" was extracted into the default plugin code: MappingTokenProfileResolver. All resolver plugin codes will extend from BaseTokenProfileResolver.

The most visible change from sites running previous versions of TPS is that the mapping related config params e.g. op.format.mapping.xxx have now being extracted into token resolver instance params e.g. tokenProfileResolver.formatMappingResolver.mapping.xxxx where "class_id" defines the plugin implementation name defined in the registry.cfg and op.format.tokenProfileResolver points to the defined resolver instance e.g. op.format.tokenProfileResolver=formatMappingResolver

A separate ticket will be filed to provide actual plugin writing instruction as well as a sample plugin.

Finally, even though this patch does not directly address https://fedorahosted.org/pki/ticket/447 RFE: Mapping tokens to tokentype, where the request was to figure out the tokenType (Token profile) by uid instead of the current mapping method, it provides the means for one to write any customized plugins.

thanks, Christina

_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

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

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.

pushed to master. Ticket #447 now closed. commit e1809a3c6df50ed2e8a35ab7108a3a057e4fcaff Author: Christina Fu <cfu@redhat.com> Date: Tue Jun 3 16:42:10 2014 -0700 TPS Token Profile Resolver Framework - part2 On 06/03/2014 09:31 PM, Endi Sukma Dewata wrote:

On 6/3/2014 6:50 PM, Christina Fu wrote:

...

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

Yeah, as I mentioned over IRC, the current UI labels are hardcoded in the html files. We don't have translation mechanism yet.

...

5. The target gets reset at beginning at each iteration, isn't that what we want?

I still think this is a problem. I opened the following ticket: https://fedorahosted.org/pki/ticket/1032

...

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.

Installation works now, and the subsystem_layout.py is fine for this purpose, but I found another problem. If you view each profile mapping in the UI now they contain no properties. This is because the profile mapping names don't match the property names:

target.Profile_Mappings.list=enrollProfileResolver,formatProfileResolver,pinResetProfileResolver

target.Profile_Mappings.pattern=tokenProfileResolver\.$name\.mapping\..*

tokenProfileResolver.list=formatMappingResolver,enrollMappingResolver,pinResetMappingResolver

tokenProfileResolver.enrollMappingResolver... tokenProfileResolver.formatMappingResolver... tokenProfileResolver.pinResetMappingResolver...

Notice the top ones end with "...ProfileResolver" and the bottom ones end with "...MappingResolver".

Once this is fixed, it's ACKed.