[Pki-devel] [PATCH] pki-cfu-0027-ticket-882-tokendb-policy-handling-revocation-and-re.patch

[Christina Fu]

Agreed to table the structure discussion (#2) to next patch - Note: I 
have converted Hashtables to ArrayList since.
Verbal ACK received from Jack.

Pushed to master:
commit f90798b725430ac2ec44d1e29ea9fbd53abc4c64

thanks,
Christina

On 08/18/2014 07:56 PM, Christina Fu wrote:
> 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 at redhat.com>
>> To: pki-devel at 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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/pki-devel
>
>
>
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20140820/3949a66b/attachment.htm>