<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Agreed to table the structure discussion (#2) to next patch - Note:
I have converted Hashtables to ArrayList since.<br>
Verbal ACK received from Jack.<br>
<br>
Pushed to master:<br>
commit f90798b725430ac2ec44d1e29ea9fbd53abc4c64<br>
<br>
thanks,<br>
Christina<br>
<br>
<div class="moz-cite-prefix">On 08/18/2014 07:56 PM, Christina Fu
wrote:<br>
</div>
<blockquote cite="mid:53F2BCE7.6000309@redhat.com" type="cite">Thanks
for the review, jack. Please find the new patch that:
<br>
1. checks for tps null value in TPSTokenPolicy constructor
<br>
2. for the methods with Hashtables passing around, I added
comments to explain what they are, and renamed the methods to be
more clear on what they do. However, I need time to think about
creating classes for them as I'm not very certain we want to do
that at the moment. I will add the discussion to the next patch,
if it's okay with you,.
<br>
3. I moved the check with revokeCert config to the top of the
method.
<br>
<br>
Finally, as I stated, there are improvements to come for the
recovery decision method.
<br>
<br>
thanks,
<br>
Christina
<br>
<br>
On 08/18/2014 07:16 PM, John Magne wrote:
<br>
<blockquote type="cite">Comments:
<br>
<br>
-This method:
<br>
<br>
public TPSTokenPolicy (TPSSubsystem tps) {
<br>
+ this.tps = tps;
<br>
+ // init from config first
<br>
+ String policySetString = getDefaultPolicySetString();
<br>
+ parsePolicySetString(policySetString);
<br>
<br>
<br>
Maybe just check for a null tps in the constructor and throw an
exception.
<br>
Then we don't have to worry about tps being invalid again.
<br>
<br>
<br>
-This method:
<br>
<br>
public Hashtable<String, TokenRecord>
tdbFindTokenRecords(String uid)
<br>
+ throws Exception {
<br>
<br>
OK, there are a bunch of places where we are passing back and
passing around these hashbables of tokens or certs or whatever.
<br>
<br>
First we have no idea what is in the the hash table unless
inspecting the code closely.
<br>
Second. It looks like this method is takin an Iterator returned
by the DB code and then making this hash table.
<br>
<br>
I propose that we create classes for this such as
"TPSTokenRecordList" and then provide convenient methods to
return the entries
<br>
to the caller. We could even just use the Iterator returned
before. Then when we pass this stuff to other functions , we
pass
<br>
the convenient class.
<br>
<br>
-This method:
<br>
<br>
+ private TPSStatus
generateCertsAfterRenewalRecoveryPolicy(EnrolledCertsInfo
certsInfo, SecureChannel channel, AppletInfo aInfo)
<br>
+ throws TPSException {
<br>
<br>
Just restating what you said earlier in that this thing is way
to complex and confusing to digest. It needs simplification.
<br>
<br>
<br>
- This method:
<br>
<br>
protected void revokeCertificates(String cuid) throws
TPSException {
<br>
<br>
<br>
Here we wait until we are like 70% of the way through the
processing to see if we are even configured to revoke
certificates.
<br>
I see that in the beginning we try to cull out already revoked
certs, but this could be done in another method or just
<br>
wait for the next time we actually need to revoke anything.
<br>
<br>
<br>
<br>
<br>
----- Original Message -----
<br>
From: "Christina Fu" <a class="moz-txt-link-rfc2396E" href="mailto:cfu@redhat.com"><cfu@redhat.com></a>
<br>
To: <a class="moz-txt-link-abbreviated" href="mailto:pki-devel@redhat.com">pki-devel@redhat.com</a>
<br>
Sent: Monday, August 18, 2014 6:56:17 PM
<br>
Subject: [Pki-devel] [PATCH]
pki-cfu-0027-ticket-882-tokendb-policy-handling-revocation-and-re.patch
<br>
<br>
This is continuation of token policy work from ticket #882
<br>
<br>
This patch provides
<br>
1. the 2nd part (and not last) of the token policy work
<br>
- determining policies for Enrollment, Re-enrollment,
Renewal,
<br>
Recovery, and Revocation
<br>
(note: Recovery decision needs more refinement in later
patch. The
<br>
generateCertsAfterRenewalRecoveryPolicy method is currently just
a close
<br>
translation from the original TPS and is extremely complex. It
deserves
<br>
some close inspection and improvement at later time, in next
round)
<br>
2. revocation for certificates due to token operations
<br>
(note: administrator initiated will be handled in later
patch)
<br>
<br>
thanks,
<br>
Christina
<br>
<br>
_______________________________________________
<br>
Pki-devel mailing list
<br>
<a class="moz-txt-link-abbreviated" href="mailto:Pki-devel@redhat.com">Pki-devel@redhat.com</a>
<br>
<a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/pki-devel">https://www.redhat.com/mailman/listinfo/pki-devel</a>
<br>
</blockquote>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
Pki-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:Pki-devel@redhat.com">Pki-devel@redhat.com</a>
<a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/pki-devel">https://www.redhat.com/mailman/listinfo/pki-devel</a></pre>
</blockquote>
<br>
</body>
</html>