[RFC][icedtea-web] Unit tests for the new updates to JarSIgner (incomplete)

[Omair Majid]

On 04/06/2012 05:57 PM, Danesh Dadachanji wrote:
> On 04/04/12 07:36 PM, Omair Majid wrote:
>> On 04/02/2012 04:46 PM, Danesh Dadachanji wrote:
>>> My understanding of the idea behind verifyJar is that it does 2 somewhat
>>> separate tasks. First, given a jar, iterate over all of its entries. For
>>> every entry that can be signed, go through the list of signers and see
>>> if any have issues. If they do, setup the flags accordingly (issues
>>> could be expiry/bad key usage/not yet valid start date etc). Also, keep
>>> track of the number of entries each signer has signed, as well as the
>>> total number of possible signable entries. The second step is going
>>> through all of the signers after all the entries have been iterated over
>>> to see if there is a common signer that has signed them all. If there is
>>> any entry that is not signed but should be, or if there is one entry
>>> that does not share a common signer between all other entries, then this
>>> jar is considered UNSIGNED. If all entries (that should be signed) share
>>> a common signer, then the signer itself is checked. If the signer has
>>> signing issues, the jar is considered SIGNED_NOT_OKAY, if it has no
>>> signing issues, the jar is considered SIGNED_OK.
>>>
>>
>> This does make sense to me. Something that sort of jumps out at me is
>> that these two responsibilities are fairly distinct. It seems like it
>> might be worth it to split it into two distinct objects, each taking
>> care of one responsibility. That is, one objects determines if it should
>> be considered signed or unsigned, and another checks that (if signed)
>> the signing certificate is good.
>>
> 
> Hmm interesting idea, it makes sense to separate the checking/setup of
> the signers and the checking of the jar. However, I would like to get
> back to you on this once I have wondered into the mess that is
> verifyJars(). I think there is definitely potential here to move bits
> into another class but I don't fully know what is needed by the rest of
> JarCertVerifier. Until I do, I would rather avoid refactoring this until
> it is needed.

Yeah, that's fine. Lets not try to do too much in one patch :)

>>> Comments are very appreciated!
>>
>> Some more whitespace might be nice (to separate the asserts from the
>> code that sets up the preconditions and code that tests the object). We
>> may also want to split off the separate responsibilities into separate
>> classes. Still, I think it looks fantastic for now. Nicely done!
> 
> Separate responsibilities WRT what exactly?
> 
> The whitespaces definitely do help! =)
> 
> I've attached a new patch that loosens up the code a bit, I've padded
> newlines above (and below) the chunk of asserts. There are a few more
> modifications as follows.
> 
> 1) The new patch also brings in the changeset that renames JarSigner to
> JarCertVerifier[1] so hopefully it won't throw reviewers off too much.

Great :)

> 2) In the tests that run 1 entry than 3 entries, I've reset
> JarCertVerifier to a new instance when the three-entry vector is used. I
> think this may cause confusion later on because there would have been
> cases where the JarCertVerifier would have totalSignableEntries = 4
> instead of 3.

Hm... If you are doing this, I would recommend you go all the way and
split the 1-entry/3-entry tests into different methods to isolate the
tests completely.

> 3) Lastly, I've added a new set of asserts using the following two methods:
> 
>     /**
>      * Checks if the provided cert path has signed any of the jars
> associated
>      * with the cert verifier.
>      * @param certPath The cert path of the potential signer.
>      * @return Whether or not the cert path represents one of the
> signers of
>      * a jar associated with the verifier.
>      */
>     boolean hasJarSigner(CertPath certPath)

The name is not very descriptive. How about something like
certHasSignedAnyJar? Also, why a certPath instead of a cert?

>     /**
>      * Calculate the number of signers all the jars have in total.
>      *
>      * Note this is not the number of common signers across all jars. It is
>      * the number of common signers each jar has with respect to itself,
>      * totalled together across the other jars.
>      * @return The number of signers that have signed at least one jar
> entirely.
>      */
>     int numSigners()
> 

Likewise, this name doesn't convey much. I am still trying to figure out
what this method does.

Are these methods public? Any reason why we are adding these (or are
they just for tests)gf?

Thanks,
Omair