Looks nice and gives us some good new flexibility with respect to the keySet value. Just a few comments: 1. For each type of "resolver" you have something like: mappingResolver.enrollMappingResolver Previously the whole class name for this was something like "tokenProfileMappingResolver" or some such. This name has been changed to just "mappingResolver". In order to give the user the same info how about something like: "mappingResolver.enollTokenTypeMappingResolver" ??? The same for format and pin reset of course. 2. In MappingResolverManager.java here: mappingResolvers.put(prInst, resolver); The mappingResolvers property is protected. To make it easier for subclasses to use this, maybe an "addResolver" method for easier use instead of making raw collection calls. 3. In the TKSRemoteRequestHandler.java we have modified a large number of method signatures in order to pass in the newly calculated "keySet" type. Instead of modifying all of the calls inside of TKSRemoteRequestHandler there I suggest this possibility. 1. When we construct the instance of TKSRemoteRequestHandler, we add a new parameter to the constructor being "keySet", which is invariant per session. Set a private property of the class to it. Also validation can be done in the constructor of this value. 2. IN all the methods where that use the new param, just use the local property and no need to worry about validation. 3. I see that the TPSEngine methods take the new param as well. It would be nicer to not have to do that, but I think it might be most convenient to leave that "keySet" param so it can be used in the constructor to TKSRemoteRequestHandler. 4. In TPSEnrollProcessor I see this block several times in different places. + + CMS.debug("In TPSEnrollProcessor.enroll isExternalReg: about to process keySet resolver"); + /* + * Note: externalReg.mappingResolver=none indicates no resolver + * plugin used + */ + try { + String resolverInstName = getKeySetResolverInstanceName(); + + if (!resolverInstName.equals("none") && (selectedKeySet == null)) { + FilterMappingParams mappingParams = createFilterMappingParams(resolverInstName, + appletInfo.getCUIDhexString(), appletInfo.getMSNString(), + appletInfo.getMajorVersion(), appletInfo.getMinorVersion()); + TPSSubsystem subsystem = + (TPSSubsystem) CMS.getSubsystem(TPSSubsystem.ID); + BaseMappingResolver resolverInst = + subsystem.getMappingResolverManager().getResolverInstance(resolverInstName); + String keySet = resolverInst.getResolvedMapping(mappingParams, "keySet"); + setSelectedKeySet(keySet); + CMS.debug(method + " resolved keySet: " + keySet); + } + } catch (TPSException e) { + auditMsg = e.toString(); + tps.tdb.tdbActivity(ActivityDatabase.OP_FORMAT, tokenRecord, session.getIpAddress(), auditMsg, + "failure"); + + throw new TPSException(auditMsg, TPSS I think this thing can be made a method of TPSProcessor or something and just called everywhere. 5. This method: protected String getKeySetResolverInstanceName() throws TPSException { + String method = "TPSProcessor.getKeySetResolverInstanceName: "; + CMS.debug(method + " begins"); + IConfigStore configStore = CMS.getConfigStore(); + .... .. + + CMS.debug(method + " config: " + config); + try { + resolverInstName = configStore.getString(config, "none"); + } catch (EBaseException e) { + // not finding it is not an error + } + if (resolverInstName.equals("")) + resolverInstName = "none"; + + CMS.debug(method + " returning: " + resolverInstName); + + return resolverInstName; + } We've established that swallowing exceptions is not a good thing to do. Just throw a TPSException because here there is some internal error, since you have already established a default. 6. After doing the above, it might be nice to just try a key changeover operation with tpsclient to make sure everything is kosher after changing the behaviour of the TKSRemoteRequestHandler slightly. thanks, jack ----- Original Message ----- > From: "Christina Fu" <cfu(a)redhat.com&gt; > To: pki-devel(a)redhat.com > Sent: Monday, May 18, 2015 5:52:20 PM > Subject: [Pki-devel] [PATCH] pki-cfu-0066 and pki-cfu-0067 for Ticket 1307 [RFE] Support multiple keySets for > different cards for ExternalReg > > Please find two patches for the ticket: > https://fedorahosted.org/pki/ticket/1307 [RFE] Support multiple keySets > for different cards for ExternalReg > > Patch pki-cfu-0066 involves only renaming of classes/methods/parameters > and the related config parameters for the Mapping Resolver framework. > (note: after the refactoring, I tested it to work before continuing to > the 2nd part) > It is separated out from the actual code logic changes for ease of review. > The renaming is necessary as the original framework was intended only > to be used to resolve tokenType, and it is now expanded to be used to > resolve keySet. > > Patch pki-cfu-0067 deals with the actual code changes that adds support > for keySet mapping > > Original design of this add-on ExternalReg feature can be found here: > http://pki.fedoraproject.org/wiki/TPS_-_New_Recovery_Option:_External_Reg... > > There is no upgrade supported at this point, as this is technology > preview feature. > > Please review. > thanks, > Christina > > _______________________________________________ > Pki-devel mailing list > Pki-devel(a)redhat.com > https://www.redhat.com/mailman/listinfo/pki-devel

Christina: Looks like you got the minor issues under control. We went through a couple of demos and it is tested to work with tpsclient... ACK ----- Original Message ----- > From: "Christina Fu" <cfu(a)redhat.com&gt; > To: pki-devel(a)redhat.com > Sent: Thursday, May 21, 2015 4:35:13 PM > Subject: Re: [Pki-devel] [PATCH] pki-cfu-0066 and pki-cfu-0067 for Ticket 1307 [RFE] Support multiple keySets for > different cards for ExternalReg > > Thanks for the review Jack. > Attached please find pki-cfu-0069, which is the revised patch#2. > > Christina > > On 05/20/2015 01:51 PM, John Magne wrote: >> Looks nice and gives us some good new flexibility with respect to the >> keySet value. >> >> >> Just a few comments: >> >> >> 1. For each type of "resolver" you have something like: >> >> mappingResolver.enrollMappingResolver >> >> Previously the whole class name for this was something like >> "tokenProfileMappingResolver" or some such. >> This name has been changed to just "mappingResolver". In order to give the >> user the same info how about >> something like: "mappingResolver.enollTokenTypeMappingResolver" ??? The >> same for format and pin reset of course. >> >> >> 2. In MappingResolverManager.java here: >> >> mappingResolvers.put(prInst, resolver); >> >> >> The mappingResolvers property is protected. To make it easier for >> subclasses to use this, >> maybe an "addResolver" method for easier use instead of making raw >> collection calls. >> >> 3. In the TKSRemoteRequestHandler.java we have modified a large number of >> method signatures in order >> to pass in the newly calculated "keySet" type. >> >> Instead of modifying all of the calls inside of TKSRemoteRequestHandler >> there I suggest this possibility. >> >> 1. When we construct the instance of TKSRemoteRequestHandler, we add a >> new parameter to the constructor being "keySet", >> which is invariant per session. Set a private property of the class to it. >> Also validation can be done in the constructor of this value. >> >> 2. IN all the methods where that use the new param, just use the local >> property and no need to worry about validation. >> >> >> 3. I see that the TPSEngine methods take the new param as well. It >> would be nicer to not have to do that, but I think it might be most >> convenient to leave that "keySet" param >> so it can be used in the constructor to TKSRemoteRequestHandler. >> >> 4. In TPSEnrollProcessor I see this block several times in different >> places. >> >> + >> + CMS.debug("In TPSEnrollProcessor.enroll isExternalReg: about >> to process keySet resolver"); >> + /* >> + * Note: externalReg.mappingResolver=none indicates no >> resolver >> + * plugin used >> + */ >> + try { >> + String resolverInstName = getKeySetResolverInstanceName(); >> + >> + if (!resolverInstName.equals("none") && (selectedKeySet == >> null)) { >> + FilterMappingParams mappingParams = >> createFilterMappingParams(resolverInstName, >> + appletInfo.getCUIDhexString(), >> appletInfo.getMSNString(), >> + appletInfo.getMajorVersion(), >> appletInfo.getMinorVersion()); >> + TPSSubsystem subsystem = >> + (TPSSubsystem) >> CMS.getSubsystem(TPSSubsystem.ID); >> + BaseMappingResolver resolverInst = >> + >> subsystem.getMappingResolverManager().getResolverInstance(resolverInstName); >> + String keySet = >> resolverInst.getResolvedMapping(mappingParams, "keySet"); >> + setSelectedKeySet(keySet); >> + CMS.debug(method + " resolved keySet: " + keySet); >> + } >> + } catch (TPSException e) { >> + auditMsg = e.toString(); >> + tps.tdb.tdbActivity(ActivityDatabase.OP_FORMAT, >> tokenRecord, session.getIpAddress(), auditMsg, >> + "failure"); >> + >> + throw new TPSException(auditMsg, TPSS >> >> >> I think this thing can be made a method of TPSProcessor or something and >> just called everywhere. >> >> 5. This method: >> >> protected String getKeySetResolverInstanceName() throws TPSException { >> + String method = "TPSProcessor.getKeySetResolverInstanceName: "; >> + CMS.debug(method + " begins"); >> + IConfigStore configStore = CMS.getConfigStore(); >> + .... >> .. >> + >> + CMS.debug(method + " config: " + config); >> + try { >> + resolverInstName = configStore.getString(config, "none"); >> + } catch (EBaseException e) { >> + // not finding it is not an error >> + } >> + if (resolverInstName.equals("")) >> + resolverInstName = "none"; >> + >> + CMS.debug(method + " returning: " + resolverInstName); >> + >> + return resolverInstName; >> + } >> >> We've established that swallowing exceptions is not a good thing to do. >> Just throw a TPSException because here there is some internal error, since >> you have >> already established a default. >> >> >> 6. After doing the above, it might be nice to just try a key changeover >> operation with tpsclient to make sure >> everything is kosher after changing the behaviour of the >> TKSRemoteRequestHandler slightly. >> >> thanks, >> jack >> >> >> >> >> ----- Original Message ----- >>> From: "Christina Fu" <cfu(a)redhat.com&gt; >>> To: pki-devel(a)redhat.com >>> Sent: Monday, May 18, 2015 5:52:20 PM >>> Subject: [Pki-devel] [PATCH] pki-cfu-0066 and pki-cfu-0067 for Ticket 1307 >>> [RFE] Support multiple keySets for >>> different cards for ExternalReg >>> >>> Please find two patches for the ticket: >>> https://fedorahosted.org/pki/ticket/1307 [RFE] Support multiple keySets >>> for different cards for ExternalReg >>> >>> Patch pki-cfu-0066 involves only renaming of classes/methods/parameters >>> and the related config parameters for the Mapping Resolver framework. >>> (note: after the refactoring, I tested it to work before continuing to >>> the 2nd part) >>> It is separated out from the actual code logic changes for ease of review. >>> The renaming is necessary as the original framework was intended only >>> to be used to resolve tokenType, and it is now expanded to be used to >>> resolve keySet. >>> >>> Patch pki-cfu-0067 deals with the actual code changes that adds support >>> for keySet mapping >>> >>> Original design of this add-on ExternalReg feature can be found here: >>> http://pki.fedoraproject.org/wiki/TPS_-_New_Recovery_Option:_External_Reg... >>> >>> There is no upgrade supported at this point, as this is technology >>> preview feature. >>> >>> Please review. >>> thanks, >>> Christina >>> >>> _______________________________________________ >>> Pki-devel mailing list >>> Pki-devel(a)redhat.com >>> https://www.redhat.com/mailman/listinfo/pki-devel > > _______________________________________________ > Pki-devel mailing list > Pki-devel(a)redhat.com > https://www.redhat.com/mailman/listinfo/pki-devel