Re: RFR(s): 8169196: [TESTBUG] Three tests from sun/net/www have undeclared dependencies
On 09/11/16 08:57, Sergei Kovalev wrote: Hi Daniel, Thank you for feedback. Fixed and verified locally. http://cr.openjdk.java.net/~skovalev/8169196/webrev.02/ Looks good - except for using a raw type at line 220: 220 Class ntlmProxyClass should be: Class ntlmProxyClass No need for a new webrev. best regards, -- daniel
Re: RFR(s): 8169196: [TESTBUG] Three tests from sun/net/www have undeclared dependencies
Hi Sergey,
This looks good now - except for line 223:
221 Field ntlmSupportedField =
ntlmProxyClass.getDeclaredField("supported");
222 ntlmSupportedField.setAccessible(true);
223 if
(Boolean.TRUE.equals(ntlmSupportedField.getBoolean(ntlmProxyClass))) {
This should be:
if (ntlmSupportedField.getBoolean(null)) {
best regards,
-- daniel
On 08/11/16 16:50, Sergei Kovalev wrote:
Hi Daniel,
All remarks incorporated into:
http://cr.openjdk.java.net/~skovalev/8169196/webrev.01/
Also I've added a note to the
https://bugs.openjdk.java.net/browse/JDK-8038079
Re: RFR(s): 8169196: [TESTBUG] Three tests from sun/net/www have undeclared dependencies
Hi Daniel,
All remarks incorporated into:
http://cr.openjdk.java.net/~skovalev/8169196/webrev.01/
Also I've added a note to the
https://bugs.openjdk.java.net/browse/JDK-8038079
--
With best regards,
Sergei
04.11.16 14:20, Daniel Fuchs wrote:
Hi Sergey,
Yes - sorry - I misunderstood the purpose of the NoNTLM test.
The test is supposed to run *only* when NTLM *is not* supported.
When NTLM is supported, it is supposed to return without doing
anything.
Now as it happens, NTLM is supported by default in java.base.
The only case where NTLM is not supported is if some profile
strip down the binaries to exclude the package
sun.net.www.protocol.http.ntlm
(which seems to be the reason why java.base uses reflection to
access the classes in sun.net.www.protocol.http.ntlm).
Basically, this means that NTLM is not supported when
sun.net.www.protocol.http.NTLMAuthenticationProxy.supported == false.
Somehow the test assumes that this will happen if and only if
the java.security.jgss is not linked in (which I think is an obsolete
assertion, because now with --limit-mods and jlink you can exclude
java.security.jgss without necessarily running on a profile
where the binaries have been stripped).
If you add @modules jdk.security.auth to test, then what will happen
is that the test will only run when jdk.security.auth is linked
in, which in turn implies that java.security.jgss is also linked in
because jdk.security.auth requires java.security.jgss.
So the test will either not be run (when jdk.security.auth is
not linked in), or return immediately without doing anything
(when jdk.security.auth is linked in) - so if you add
@modules jdk.security.auth to test, the test serves no purpose
and it's equivalent to simply delete it.
So what you need to do here is replace:
218 // assume NTLM is not supported when Kerberos is not
available
219 try {
220 Class.forName("javax.security.auth.kerberos.KerberosPrincipal");
221 System.out.println("Kerberos is present, assuming
NTLM is supported too");
222 return;
223 } catch (ClassNotFoundException okay) { }
with something that mimics what java.base does to figure whether
NTLM is supported:
load sun.net.www.protocol.http.NTLMAuthenticationProxy,
through reflection (use the 3 args Class.forName to make
sure the static initializers are run), then use reflection
to access the "supported" field of that class, make it
accessible (Field.setAccessible), get its value,
and return if its value is true.
We need to use reflection here because NTLMAuthenticationProxy
is a package class (it is not public).
Then instead of adding @module jdk.security.auth which is a
nonsense, add @module java.base/sun.net.www.protocol.http
to grant the test the power to break in through the strong
encapsulation for testing purposes.
It would also probably be good to log a new defect asking
for this code to be revisited - or drop a note in
https://bugs.openjdk.java.net/browse/JDK-8038079
pointing to this test and mentioning that a better
entry point to figure out whether NTLM is supported or not
might be desirable.
best regards,
-- daniel
On 03/11/16 15:43, Sergei Kovalev wrote:
Daniel,
I case I remove lines 218-223 and disable jdk.security.auth module the
test fails with an Exception:
Expect client to choose: Basic
HTTP/1.1 401 Unauthorized
Content-Length: 0
Connection: close
WWW-Authenticate: Basic realm="wallyworld"
WWW-Authenticate: NTLM
Server received Authorization header: NTLM
TlRMTVNTUAABB4IIAAA=
STDERR:
java.lang.RuntimeException: Unexpected value
at NoNTLM.test(NoNTLM.java:174)
at NoNTLM.main(NoNTLM.java:229)
at
jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(java.base@9-ea/Native
Method)
at
jdk.internal.reflect.NativeMethodAccessorImpl.invoke(java.base@9-ea/NativeMethodAccessorImpl.java:62)
at
jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(java.base@9-ea/DelegatingMethodAccessorImpl.java:43)
at
java.lang.reflect.Method.invoke(java.base@9-ea/Method.java:537)
at
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:110)
at java.lang.Thread.run(java.base@9-ea/Thread.java:844)
JavaTest Message: Test threw exception: java.lang.RuntimeException:
Unexpected value
JavaTest Message: shutting down test
Looks like the module also requires for www authentication in other pice
of code.
Hi Sergey,
Great to get rid of yet another shell script.
The change looks good in general - except for NoNTLM.java:
I don't think the test should have @modules jdk.security.auth
as the test is precisely supposed to be able to run without it
(+ lines 218-223 are probably obsolete and I suspect they should
be removed - but that is for another day).
best regards,
-- daniel
On 03/11/16 13:48, Sergei Kovalev wrote:
Hi Team,
Please review one more small fix for module dependencies issue.
Bug ID:
Re: RFR(s): 8169196: [TESTBUG] Three tests from sun/net/www have undeclared dependencies
Hi Sergey,
Yes - sorry - I misunderstood the purpose of the NoNTLM test.
The test is supposed to run *only* when NTLM *is not* supported.
When NTLM is supported, it is supposed to return without doing
anything.
Now as it happens, NTLM is supported by default in java.base.
The only case where NTLM is not supported is if some profile
strip down the binaries to exclude the package
sun.net.www.protocol.http.ntlm
(which seems to be the reason why java.base uses reflection to
access the classes in sun.net.www.protocol.http.ntlm).
Basically, this means that NTLM is not supported when
sun.net.www.protocol.http.NTLMAuthenticationProxy.supported == false.
Somehow the test assumes that this will happen if and only if
the java.security.jgss is not linked in (which I think is an obsolete
assertion, because now with --limit-mods and jlink you can exclude
java.security.jgss without necessarily running on a profile
where the binaries have been stripped).
If you add @modules jdk.security.auth to test, then what will happen
is that the test will only run when jdk.security.auth is linked
in, which in turn implies that java.security.jgss is also linked in
because jdk.security.auth requires java.security.jgss.
So the test will either not be run (when jdk.security.auth is
not linked in), or return immediately without doing anything
(when jdk.security.auth is linked in) - so if you add
@modules jdk.security.auth to test, the test serves no purpose
and it's equivalent to simply delete it.
So what you need to do here is replace:
218 // assume NTLM is not supported when Kerberos is not available
219 try {
220
Class.forName("javax.security.auth.kerberos.KerberosPrincipal");
221 System.out.println("Kerberos is present, assuming NTLM
is supported too");
222 return;
223 } catch (ClassNotFoundException okay) { }
with something that mimics what java.base does to figure whether
NTLM is supported:
load sun.net.www.protocol.http.NTLMAuthenticationProxy,
through reflection (use the 3 args Class.forName to make
sure the static initializers are run), then use reflection
to access the "supported" field of that class, make it
accessible (Field.setAccessible), get its value,
and return if its value is true.
We need to use reflection here because NTLMAuthenticationProxy
is a package class (it is not public).
Then instead of adding @module jdk.security.auth which is a
nonsense, add @module java.base/sun.net.www.protocol.http
to grant the test the power to break in through the strong
encapsulation for testing purposes.
It would also probably be good to log a new defect asking
for this code to be revisited - or drop a note in
https://bugs.openjdk.java.net/browse/JDK-8038079
pointing to this test and mentioning that a better
entry point to figure out whether NTLM is supported or not
might be desirable.
best regards,
-- daniel
On 03/11/16 15:43, Sergei Kovalev wrote:
Daniel,
I case I remove lines 218-223 and disable jdk.security.auth module the
test fails with an Exception:
Expect client to choose: Basic
HTTP/1.1 401 Unauthorized
Content-Length: 0
Connection: close
WWW-Authenticate: Basic realm="wallyworld"
WWW-Authenticate: NTLM
Server received Authorization header: NTLM
TlRMTVNTUAABB4IIAAA=
STDERR:
java.lang.RuntimeException: Unexpected value
at NoNTLM.test(NoNTLM.java:174)
at NoNTLM.main(NoNTLM.java:229)
at
jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(java.base@9-ea/Native
Method)
at
jdk.internal.reflect.NativeMethodAccessorImpl.invoke(java.base@9-ea/NativeMethodAccessorImpl.java:62)
at
jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(java.base@9-ea/DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(java.base@9-ea/Method.java:537)
at
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:110)
at java.lang.Thread.run(java.base@9-ea/Thread.java:844)
JavaTest Message: Test threw exception: java.lang.RuntimeException:
Unexpected value
JavaTest Message: shutting down test
Looks like the module also requires for www authentication in other pice
of code.
Hi Sergey,
Great to get rid of yet another shell script.
The change looks good in general - except for NoNTLM.java:
I don't think the test should have @modules jdk.security.auth
as the test is precisely supposed to be able to run without it
(+ lines 218-223 are probably obsolete and I suspect they should
be removed - but that is for another day).
best regards,
-- daniel
On 03/11/16 13:48, Sergei Kovalev wrote:
Hi Team,
Please review one more small fix for module dependencies issue.
Bug ID: https://bugs.openjdk.java.net/browse/JDK-8169196
WebRev: http://cr.openjdk.java.net/~skovalev/8169196/webrev.00/index.html
Added missed dependency on jdk.httpserver. Also shell test converted to
pure java test.
Re: RFR(s): 8169196: [TESTBUG] Three tests from sun/net/www have undeclared dependencies
Daniel, I case I remove lines 218-223 and disable jdk.security.auth module the test fails with an Exception: Expect client to choose: Basic HTTP/1.1 401 Unauthorized Content-Length: 0 Connection: close WWW-Authenticate: Basic realm="wallyworld" WWW-Authenticate: NTLM Server received Authorization header: NTLM TlRMTVNTUAABB4IIAAA= STDERR: java.lang.RuntimeException: Unexpected value at NoNTLM.test(NoNTLM.java:174) at NoNTLM.main(NoNTLM.java:229) at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(java.base@9-ea/Native Method) at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(java.base@9-ea/NativeMethodAccessorImpl.java:62) at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(java.base@9-ea/DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(java.base@9-ea/Method.java:537) at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:110) at java.lang.Thread.run(java.base@9-ea/Thread.java:844) JavaTest Message: Test threw exception: java.lang.RuntimeException: Unexpected value JavaTest Message: shutting down test Looks like the module also requires for www authentication in other pice of code. -- With best regards, Sergei 03.11.16 18:30, Daniel Fuchs wrote: Hi Sergey, Great to get rid of yet another shell script. The change looks good in general - except for NoNTLM.java: I don't think the test should have @modules jdk.security.auth as the test is precisely supposed to be able to run without it (+ lines 218-223 are probably obsolete and I suspect they should be removed - but that is for another day). best regards, -- daniel On 03/11/16 13:48, Sergei Kovalev wrote: Hi Team, Please review one more small fix for module dependencies issue. Bug ID: https://bugs.openjdk.java.net/browse/JDK-8169196 WebRev: http://cr.openjdk.java.net/~skovalev/8169196/webrev.00/index.html Added missed dependency on jdk.httpserver. Also shell test converted to pure java test.
Re: RFR(s): 8169196: [TESTBUG] Three tests from sun/net/www have undeclared dependencies
Hi Sergey, Great to get rid of yet another shell script. The change looks good in general - except for NoNTLM.java: I don't think the test should have @modules jdk.security.auth as the test is precisely supposed to be able to run without it (+ lines 218-223 are probably obsolete and I suspect they should be removed - but that is for another day). best regards, -- daniel On 03/11/16 13:48, Sergei Kovalev wrote: Hi Team, Please review one more small fix for module dependencies issue. Bug ID: https://bugs.openjdk.java.net/browse/JDK-8169196 WebRev: http://cr.openjdk.java.net/~skovalev/8169196/webrev.00/index.html Added missed dependency on jdk.httpserver. Also shell test converted to pure java test.
