Thanks
for the review, jack. Please find the new patch that:
1. checks for tps null value in TPSTokenPolicy constructor
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,.
3. I moved the check with revokeCert config to the top of the
method.
Finally, as I stated, there are improvements to come for the
recovery decision method.
thanks,
Christina
On 08/18/2014 07:16 PM, John Magne wrote:
Comments:
-This method:
public TPSTokenPolicy (TPSSubsystem tps) {
+ this.tps = tps;
+ // init from config first
+ String policySetString = getDefaultPolicySetString();
+ parsePolicySetString(policySetString);
Maybe just check for a null tps in the constructor and throw an
exception.
Then we don't have to worry about tps being invalid again.
-This method:
public Hashtable<String, TokenRecord>
tdbFindTokenRecords(String uid)
+ throws Exception {
OK, there are a bunch of places where we are passing back and
passing around these hashbables of tokens or certs or whatever.
First we have no idea what is in the the hash table unless
inspecting the code closely.
Second. It looks like this method is takin an Iterator returned
by the DB code and then making this hash table.
I propose that we create classes for this such as
"TPSTokenRecordList" and then provide convenient methods to
return the entries
to the caller. We could even just use the Iterator returned
before. Then when we pass this stuff to other functions , we
pass
the convenient class.
-This method:
+ private TPSStatus
generateCertsAfterRenewalRecoveryPolicy(EnrolledCertsInfo
certsInfo, SecureChannel channel, AppletInfo aInfo)
+ throws TPSException {
Just restating what you said earlier in that this thing is way
to complex and confusing to digest. It needs simplification.
- This method:
protected void revokeCertificates(String cuid) throws
TPSException {
Here we wait until we are like 70% of the way through the
processing to see if we are even configured to revoke
certificates.
I see that in the beginning we try to cull out already revoked
certs, but this could be done in another method or just
wait for the next time we actually need to revoke anything.
----- Original Message -----
From: "Christina Fu" <cfu@redhat.com>
To: pki-devel@redhat.com
Sent: Monday, August 18, 2014 6:56:17 PM
Subject: [Pki-devel] [PATCH]
pki-cfu-0027-ticket-882-tokendb-policy-handling-revocation-and-re.patch
This is continuation of token policy work from ticket #882
This patch provides
1. the 2nd part (and not last) of the token policy work
- determining policies for Enrollment, Re-enrollment,
Renewal,
Recovery, and Revocation
(note: Recovery decision needs more refinement in later
patch. The
generateCertsAfterRenewalRecoveryPolicy method is currently just
a close
translation from the original TPS and is extremely complex. It
deserves
some close inspection and improvement at later time, in next
round)
2. revocation for certificates due to token operations
(note: administrator initiated will be handled in later
patch)
thanks,
Christina
_______________________________________________
Pki-devel mailing list
Pki-devel@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
_______________________________________________
Pki-devel mailing list
Pki-devel@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel