Re: [Pki-devel] [PATCH] Ticket #888 - TPS rewrite: provide remote authority functions (patch 1: TKS functions)

On 3/11/2014 9:13 PM, Christina Fu wrote: > This is a request for review for the attached patch. > The patch is the first part for https://fedorahosted.org/pki/ticket/888 > - TPS rewrite: provide remote authority functions > > This patch provides functions for TKS: > - computeSessionKey > - createKeySetData > - encryptData > - computeRandomData > > Two things to note: > * Because of code not yet available, only computeRandomData() was > tested. Other functions can be tested/adusted when the needed info are > available. > * Because we don't know where the tps profile name will be at this > point, computeSessionKey currently has a hard-coded entry in there. It > is noted in a TODO comment. Again, this will be adjusted when it's more > clear where it comes from. > > thanks, > Christina Some comments: 1. In RemoteRequestHandler.parserResponse() now if the content is null the method will return null too. I suppose content should never be null, so this is an error case. As I mentioned in the previous review, if an error is detected, it would be better to throw an exception instead of returning null so that it can be traced to the actual error. if (content == null) { throw new EBaseException("RemoteRequestHandler: parserResponse(): no response content."); } On the other hand, if null content is considered a valid input (meaning empty content), it would be better to return an empty hashtable so the caller doesn't need to worry about null hashtable: Hashtable<String, Object> vars = new Hashtable<String, Object>(); if (content == null) { return vars; } 2. In RemoteRequestHandler.parserResponse() it's not necessary to create a new string nvs from string e: for (String e : elements) { String nvs = new String(e); String[] nv = nvs.split(NAME_VALUE_EQUAL); ... } Java strings are immutable and the split() will create new strings, so the original content won't be affected. The code can be simplified as follows: for (String nvs : elements) { String[] nv = nvs.split(NAME_VALUE_EQUAL); ... } 3. In RemoteRequestHandler.parserResponse() the following null checking is unnecessary because split() doesn't return null string elements. String[] nv = nvs.split(NAME_VALUE_EQUAL); if (nv[0] != null && nv[1] != null) { vars.put(nv[0], nv[1]); } Instead, we should check whether the array contains 2 elements. What should we do if it contains more, or less, than 2 elements? if (nv.length == 2) { vars.put(nv[0], nv[1]); } else { // throw error, skip, or ignore the extra elements? } 4. Since both the name and the value are strings, RemoteRequestHandler.parserResponse() can actually return a Hashtable<String, String> so it's not necessary to downcast the values into string again later. 5. Similar to #1, in TKSRemoteRequestHandler.computeSessionKey() if the TKS response is null it's an error case, so it should throw an exception instead of returning null again. Also note the extra parentheses in the if-statement can be removed. String content = resp.getContent(); if (content != null && !content.equals("")) { ... } else { throw new EBaseException("TKSRemoteRequestHandler: computeSessionKey(): no response content."); } 6. Regardless of #1, the following null response checking in TKSRemoteRequestHandler.computeSessionKey() is unnecessary because the response will never be null since parseResponse() will never receive a null content: if (content != null && !content.equals("")) { // content can't be null Hashtable<String, Object> response = parseResponse(content); // response can't be null if (response == null) { CMS.debug("TKSRemoteRequestHandler: computeSessionKey(): parseResponse returned null."); return null; } 7. Similar to #1, if missing status is considered an error then it should throw an exception: String value = response.get(TKS_RESPONSE_STATUS); if (value == null) { throw new EBaseException("TKSRemoteRequestHandler: computeSessionKey(): Bad TKS Connection. Please make sure TKS is accessible by TPS."); } else { CMS.debug("TKSRemoteRequestHandler: computeSessionKey(): got status =" + value); } 8. The computeSessionKey() now returns a hashtable containing attributes, so someone calling this method will have to check the docs to make sure he/she uses the right attributes and casts the values to the right types. It would be better to define a class for the return value: CreateSessionKeyResponse response = tksReq.computeSessionKey(...); String status = response.getStatus(); byte[] sessionKey = response.getSessionKey(); 9. Comments #5-#8 also apply to createKeySetData(), computeRandomData(), and encryptData().