Tests are good. Several comments: 1. Try run something like "hg mv -A" to let Mercurial knows that you are renaming files (SecurityUtils.java and those in login/modules/src) instead of removing some old and creating some new. The current webrev does not show this.
2. It's not a good practice extending a class just to use its static fields/methods. Instead, make the util class final. public class JaasModularClientTest extends SecurityUtils public class SecurityProviderModularTest extends SecurityUtils In fact, if you do find similarities between SecurityProviderModularTest and JaasModularClientTest, try abstract them into a ModularTest class. 3. Please add enough comments to SecurityUtils since it will be reused. For example, move the descriptions of EXPLICIT, AUTO and UNNAMED from test @summary to definition of enum MODULE_TYPE. Thanks Max > On Dec 20, 2015, at 2:10 PM, Sibabrata Sahoo <sibabrata.sa...@oracle.com> > wrote: > > Hi Mandy/Max, > > Here is the updated webrev with very minor change over previous: > http://cr.openjdk.java.net/~asmotrak/siba/8130360/webrev.04/ > > Thanks > Siba > > -----Original Message----- > From: Sibabrata Sahoo > Sent: Sunday, December 20, 2015 1:34 AM > To: Mandy Chung; Weijun Wang > Cc: Valerie Peng; jigsaw-dev@openjdk.java.net; security-...@openjdk.java.net > Subject: RE: [9] RFR:8130360: Add tests to verify 3rd party security > providers if they are in signed/unsigned modular JARs > > Hi Mandy/Max, > > Please review the updated webrev: > http://cr.openjdk.java.net/~asmotrak/siba/8130360/webrev.03/ > > "3rd party security providers" Test change includes: > - Test are converted to use TestNG framework. > - java/security/modules/JigsawSecurityUtils.java, renamed to > "SecurityUtils.java". Proper care required while pushing the change because > "JigsawSecurityUtils.java" is an existing file in JAKE repository. > - Common reusable operations moved to "SecurityUtils.java" > - All the following comments associated. > > > Based on the recent comments, I also did some changes to the "JAAS Modular > test" because both test are very similar to each other. The change includes: > - Flat folder structure. It places the Test as well as all its dependency in > a single folder. i.e. "javax/security/auth/login/modules". That also means > "javax/security/auth/login/modules/src/" and all its subfolders need to be > removed. > - Test converted to use TestNG framework. > > Thanks, > Siba > > -----Original Message----- > From: Mandy Chung > Sent: Wednesday, December 09, 2015 11:55 PM > To: Sibabrata Sahoo > Cc: Valerie Peng; jigsaw-dev@openjdk.java.net; security-...@openjdk.java.net > Subject: Re: [9] RFR:8130360: Add tests to verify 3rd party security > providers if they are in signed/unsigned modular JARs > > Some high-level comment: > 1. I suggest to rename JigsawSecurityUtils.java to SecurityUtils.java. The > Jigsaw prefix is not needed. > 2. SecurityProviderModularTest does the setup and launch the test with a set > of different settings. I suggest to convert it to TestNG and you can > reference the existing test/jdk/jigsaw tests which are a driver test that > does the compilation and setup with @BeforeTest and @Test for each run test > together with @DataProvider. [1] is a simple test for you to get started. > > Some comments on JigsawSecurityUtils.java: > 71 Arrays.asList(options).stream().forEach(o -> command.append(SPACE > + o)); > > this can be replaced with > Arrays.stream(options).collect(Collectors.joining(SPACE)); > > 111 System.out.println(String.format( > > You can do: System.out.format(….) instead; > > Mandy > [1] > http://hg.openjdk.java.net/jigsaw/jake/jdk/file/c1d583efa466/test/jdk/jigsaw/reflect/Proxy/ProxyTest.java > >> On Dec 9, 2015, at 10:02 AM, Sibabrata Sahoo <sibabrata.sa...@oracle.com> >> wrote: >> >> Hi Valerie, >> >> Here is the updated webrev: >> http://cr.openjdk.java.net/~ntv/siba/webrev.00/ >> >> Changes applied as per previous comments. >> - File names are modified and do not include the term JCE. >> - Source folder structure is flat now and all belongs to >> "java/security/Provider" folder. >> - Sign jar functionality removed. >> >> Thanks, >> Siba >> >> -----Original Message----- >> From: Valerie Peng >> Sent: Wednesday, December 09, 2015 5:37 AM >> To: Sibabrata Sahoo >> Cc: jigsaw-dev@openjdk.java.net; security-...@openjdk.java.net; Alan >> Bateman >> Subject: Re: [9] RFR:8130360: Add tests to verify 3rd party security >> providers if they are in signed/unsigned modular JARs >> >> Hi Siba, >> >> Here are some nits: >> >> I think it's somewhat misleading to use the term JCE here as what you are >> testing here is just security provider loading. JCE is more about security >> providers supporting export-controlled services/algorithms. >> Since your provider is just an empty one, I don't think u need to sign it >> (again, it's only for JCE providers). >> You can remove a lot of code as a result. >> Also, your directory seems a bit deep, e.g. >> modules/src/jceprovidermodule/provider/TestJCEProvider.java vs >> modules/src/jceclientmodule/provider/TestJCEProvider.java. Can all of these >> files be under the same directory instead of spreading over several level of >> subdirectories? The file names are different enough to tell which is which. >> Other regression tests for provider loading functionality are under >> test/java/security/Provider. Easier to find this way. >> >> Can you please make the appropriate changes, e.g. don't use the term JCE, >> get rid of the signing, and simplify the directory structure if possible? >> >> Thanks, >> Valerie >> >> On 12/8/2015 2:04 PM, Valerie Peng wrote: >>> Right, that'd be my expectation as well. Sounds like everything works. >>> I will change to look at your latest webrev. >>> Valerie >>> >>> On 12/8/2015 6:09 AM, Sibabrata Sahoo wrote: >>>> Hi Valerie, >>>> >>>> Here is the updated webrev: >>>> http://cr.openjdk.java.net/~ralexander/8130360/webrev.00/ >>>> >>>> Now the modular behavior for the test works as per expectation >>>> through JAKE build with the following condition. >>>> If the provider jar is available under ModulePath then the >>>> "java.security" file should have the provider configuration entry as >>>> ProviderName while in case of ClassPath the entry should hold the >>>> full class name. >>>> >>>> Thanks, >>>> Siba >>>> >>>> -----Original Message----- >>>> From: Valerie Peng >>>> Sent: Tuesday, December 08, 2015 4:46 AM >>>> To: Sibabrata Sahoo >>>> Cc: Alan Bateman; security-...@openjdk.java.net; >>>> jigsaw-dev@openjdk.java.net >>>> Subject: Re: [9] RFR:8130360: Add tests to verify 3rd party security >>>> providers if they are in signed/unsigned modular JARs >>>> >>>> Siba, >>>> >>>> I have just started to review this webrev and not done yet. >>>> As for your question, the java.security file in OpenJDK9 still uses >>>> the provider class names instead of provider names. Are you talking >>>> about the java.security file in Jigsaw? Which build (OpenJDK or >>>> Jigsaw) have you tested against. I am pretty sure that I tested the >>>> 3rd party provider on classpath setting when I worked on the 7191662 >>>> changes. >>>> >>>> Supposedly, if the jar files are available on class path, then you >>>> should specify its full *class name* in the java.security file for >>>> it to be instantiated. Otherwise, how would it find the class? Only >>>> the classes discovered by ServiceLoader can be identified using the >>>> provider name (which is different from the class name referred >>>> above). So, in your scenario, that would be >>>> "provider.TestJCEProvider" instead of "TEST". >>>> >>>> If you still run into problems, try enable the java security debug >>>> flag and u should get a good idea why it isn't loaded. Let me know >>>> if you still have questions. >>>> >>>> Thanks, >>>> Valerie >>>> >>>> On 11/30/2015 6:47 AM, Sibabrata Sahoo wrote: >>>>> I would like to know more about this. As far, I can see the >>>>> "java.security" provider configuration available with JDK9, it >>>>> holds provider names instead of provider class names. In that case >>>>> how it resolve the fall back? >>>>> >>>>> Thanks, >>>>> Siba >>>>> >>>>> -----Original Message----- >>>>> From: Alan Bateman >>>>> Sent: Monday, November 30, 2015 4:54 PM >>>>> To: Sibabrata Sahoo; security-...@openjdk.java.net; >>>>> jigsaw-dev@openjdk.java.net >>>>> Subject: Re: [9] RFR:8130360: Add tests to verify 3rd party >>>>> security providers if they are in signed/unsigned modular JARs >>>>> >>>>> On 30/11/2015 11:13, Sibabrata Sahoo wrote: >>>>>> Here is the updated webrev: >>>>>> http://cr.openjdk.java.net/~asmotrak/siba/8130360/webrev.02/ >>>>>> >>>>>> I have one question: >>>>>> What should be the behavior when the older version of 3rd party >>>>>> JCE provider jar file(without service descriptor >>>>>> "META-INF/services/*"& working with<= JDK8) configured by >>>>>> "java.security" file, will be place in CLASS_PATH, running through >>>>>> JDK9 and the client is using Security.getProvider() to look for >>>>>> the provider? >>>>>> >>>>>> Currently the scenario fails to find the JCE provider. Is this >>>>>> right behavior? If it is, then jdk9 is not backward compatible to >>>>>> find the security provider provided through older jar files from >>>>>> CLASS_PATH. >>>>>> >>>>> The JCE work in JDK 9 (via JDK-7191662) was meant to address this >>>>> point by falling back and attempting to load the class name >>>>> specified via the security.provider.<N> properties in the >>>>> java.security file. I'm sure Valerie can say more about this. >>>>> >>>>> -Alan >
