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

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