Re: [Pki-devel] [PATCH] pki-cfu-0066 and pki-cfu-0067 for Ticket 1307 [RFE] Support multiple keySets for different cards for ExternalReg

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 -----