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@redhat.com>
To: pki-devel@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_Registration_DS#Supporting_multiple_keySets_for_different_cards_for_ExternalReg
There is no upgrade supported at this point, as this is technology
preview feature.
Please review.
thanks,
Christina
_______________________________________________
Pki-devel mailing list
Pki-devel@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
pushed to master
commit fe9e2d9a677317585db34ac5131d17f696c1e09e Author: Christina Fu <cfu@…> Date: Mon May 18 16:14:47 2015 -0700
commit 2e6537e80d42c208a96e218d84ed4fb5c6b7a9d4 Author: Christina Fu <cfu@…> Date: Wed May 13 08:35:34 2015 -0700