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

[Christina Fu]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-cfu-0028-ticket-882-tokendb-policy-handling-revocation-and-re.patch
Type: text/x-patch
Size: 96080 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20140818/0e637c2e/attachment.bin>