RFR 8042900: Allow com.sun.security.jgss to be in different module than org.ietf.jgss

[Wang Weijun]

Hi Alan

Thanks for the review. All suggestions accepted.

Change for S4U2selfGSS is not related and can be reverted.

In fact, I have a question. Is there a way to run a test with a module invisible? I can try my tests by removing the com.sun.security.jgss classes manually but that won't work for automatic regression tests.

--Max


On Aug 28, 2014, at 16:54, Alan Bateman <Alan.Bateman at oracle.com> wrote:

> On 27/08/2014 08:17, Wang Weijun wrote:
>> Hi All
>> 
>> Please review the code change at
>> 
>>    http://cr.openjdk.java.net/~weijun/8042900/webrev.00/
>> 
>> The purpose of this fix is to move com.sun.security.jgss (where the ExtendedGSSXXX classes reside) out of the java.security.jgss module, since it is openjdk-specific and not defined in Java SE. This is listed as the 1st open issue in http://openjdk.java.net/jeps/200.
>> 
>> Because GSSManager::createContext() actually returns an ExtendedGSSContext object now, we will need to add a check there so that when the extended classes are available it still returns ExtendedGSSContext and otherwise a plain GSSContext. An internal OrgIetfJgssExtender class is defined and Extender in com.sun.security.jgss extends it. When GSSManagerImpl is creating a GSSContext (or GSSCredential) object, it detects whether an extension is available and if so returns a "wrapped" object that has extended functions.
>> 
>> Please note that in this fix all extended functions are actually implemented in the basic (say, GSSContextImpl) class, but they are only exported through the extended interface (say, ExtendedGSSContext) to application developers.
>> 
>> The detection of whether an extension is available is now performed by calling "Class.forName("com.sun.security.jgss.Extender")". This will be revisited later if we have other handy methods to detect whether a module is available or be converted to a service loader.
>> 
>> A sub-task (JDK-8056141) will move com.sun.security.jgss and com.sun.security.sasl.gsskerb into a new jdk.security.jgss module.
>> 
>> 
> I've looked through the changes, I don't claim to know this area very well and so I think it needs a Reviewer from the security area to help review too.
> 
> At a high-level then the approach seems reasonable and I see how this will fits with the second part that will facilitate the refactoring of com.sun.security.jgss to another module. The Class.forName to tickle the Extender to register seems okay.
> 
> A minor comment on Extender is that I think I'd prefer the methods to named "wrap" rather than "wrapped", also I assume you'll add a copyright header to the file before you push.
> 
> A minor comment on OrgIetfJgssExtender is that perhaps JgssExtender would be a simpler name. Can the setExtender method be protected rather than public, I assume it will only ever be called by sub-types. Should theOthe be volatile? I can't tell if there are any race conditions here. As per Extender I would have a preference for wrap rather than wrapped.
> 
> I don't think I understand the update to test S4U2selfGSS.java, is this related to this change?
> 
> Otherwise I don't see any issues and overall this seems good work.
> 
> -Alan.